Fix CachingService.TryGetValue returning wrong value #177
Conversation
| return CacheProvider.TryGetValue(key, out value); | ||
| try | ||
| { | ||
| var contains = CacheProvider.TryGetValue<Lazy<T>>(key, out var lazyFactory); |
There was a problem hiding this comment.
Could you use TryGetValue and then do something to coerce the value, instead of relying on catching cast exceptions? Also, as a larger issue, instead of storing both Lazy and non-Lazy values in CacheProvider, what if you just always stored a Lazy? That way you don't have to figure out what is in there when retrieving the values.
| try | ||
| { | ||
| var contains = CacheProvider.TryGetValue<Lazy<T>>(key, out var lazyFactory); | ||
| value = GetValueFromLazy<T>(lazyFactory, out var _); | ||
| return contains; | ||
| } | ||
| catch (InvalidCastException) | ||
| { | ||
| return CacheProvider.TryGetValue<T>(key, out value); | ||
| } |
There was a problem hiding this comment.
| try | |
| { | |
| var contains = CacheProvider.TryGetValue<Lazy<T>>(key, out var lazyFactory); | |
| value = GetValueFromLazy<T>(lazyFactory, out var _); | |
| return contains; | |
| } | |
| catch (InvalidCastException) | |
| { | |
| return CacheProvider.TryGetValue<T>(key, out value); | |
| } | |
| if (CacheProvider.TryGetValue(key, out object item)) | |
| { | |
| value = GetValueFromLazy<T>(item, out _); | |
| return true; | |
| } | |
| value = default; | |
| return false; |
There was a problem hiding this comment.
@joaompneves - Given that CacheProvider can still contain multiple types, I think you have made the right change, here. I still think this whole implementation would be a lot cleaner if it only put Lazy in CacheProvider, so you knew to only expect Lazy when pulling values out. I'm not sure how to do that and continue to support AsyncLazy, though.
There was a problem hiding this comment.
I understand your ideas, sound standard popular known best practices. I think with this specific scenario. It will introduce extra overhead Lazy types everywhere, which the current PR approach it also won't break many existing implement ation and clients. What do you think ?
There was a problem hiding this comment.
@Coder3333 If you look at the GetValueFromLazy already handles the async lazy scenario in a sync way. In fact I just used a similar approach that the Getdoes.
Changes
related with #176